feat(runtime): restrict creation of non-implicit TLA #9589
Conversation
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Looks like Jakob's approval expired :( @akhi3030 can I get a stamp of the PR? |
|
|
||
| nightly = [ | ||
| "nightly_protocol", | ||
| "protocol_feature_fix_contract_loading_cost", | ||
| "protocol_feature_fix_staking_threshold", | ||
| "protocol_feature_reject_blocks_with_outdated_protocol_version", | ||
| "protocol_feature_restrict_tla", |
There was a problem hiding this comment.
nit: I don't think tla is a widely known acronym, how about the full wording "top_level_accounts"? Same in the protocol feature enum.
| #[cfg(feature = "protocol_feature_restrict_tla")] | ||
| ProtocolFeature::RestrictTla => 139, |
There was a problem hiding this comment.
nit: Out of order, probably due to some merging, can you put it after post state root?
Any reason to increase the version by 3?
There was a problem hiding this comment.
nit: Out of order, probably due to some merging, can you put it after post state root?
Done
Any reason to increase the version by 3?
That is because there was a 138 when this feature was implemented and it is actually hard to change because of the yaml file (a bunch of things need to be regenerated). There is no harm jumping a few versions for nightly so I think it's okay.
| @@ -0,0 +1,2 @@ | |||
| # Implements NEP-492, disallowing all top-level accounts. | |||
There was a problem hiding this comment.
TIL
Also cool that it respects protocol version.
| env.clients[0].chain.get_final_transaction_result(&tx_hash).unwrap(); | ||
| assert_eq!( | ||
| transaction_result.status, | ||
| FinalExecutionStatus::Failure( |
There was a problem hiding this comment.
I'm a bit confused that all of the accounts failed to be created. Are the hex accounts above not implicit accounts? Can you add a comment or adjust the name of the test to reflect that it's testing this NEP specifically?
| let account: AccountId = "test0".parse().unwrap(); | ||
| let mut genesis = Genesis::test(vec![account.clone()], 1); | ||
| genesis.config.epoch_length = epoch_length; | ||
| genesis.config.protocol_version = PROTOCOL_VERSION; |
There was a problem hiding this comment.
Hmm shouldn't that be nightly or the exact protocol version for this NEP, at least until this feature is stabilized?
There was a problem hiding this comment.
The test is only enabled for nightly. This is done in feature.rs with conditional compilation
| FinalExecutionStatus::Failure( | ||
| ActionError { | ||
| index: Some(0), | ||
| kind: ActionErrorKind::CreateAccountOnlyByRegistrar { |
There was a problem hiding this comment.
nit: Can you add a dedicated error type for the restricted TLAs? OnlyByRegistrar seems to be about something else.
There was a problem hiding this comment.
No it is the same thing. The purpose of registrar is to create top level accounts
| #[allow(unused)] | ||
| pub(crate) fn create_account( |
There was a problem hiding this comment.
Why do you need this function? I think it's better to only add it when needed to reduce code bloat.
There was a problem hiding this comment.
Switched to conditional compilation. Hopefully that is more clear
Stabilize near/NEPs#492. Restrict the creation of non-implicit top-level account that are longer than 32 bytes. Only the registrar account can create them. More context on the proposal: - The NEP has been officially approved in September (near/NEPs#492 (comment)). - #9589 implements the feature and includes tests checking that TLA can no longer be created in the new protocol version.
Stabilize near/NEPs#492. Restrict the creation of non-implicit top-level account that are longer than 32 bytes. Only the registrar account can create them. More context on the proposal: - The NEP has been officially approved in September (near/NEPs#492 (comment)). - #9589 implements the feature and includes tests checking that TLA can no longer be created in the new protocol version.
Implements near/NEPs#492